-
Notifications
You must be signed in to change notification settings - Fork 1k
Update link agreement to show compliance text #1850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update link agreement to show compliance text #1850
Conversation
Co-authored-by: marcftone <[email protected]>
Cursor Agent can help with this pull request. Just |
WalkthroughAdds content-type support for agreements (LINK or TEXT) across UI, API, and database; updates agreement creation UI and payload shape; forwards contentType through view components to render text vs link; adds Zod validation and sanitization in the agreements API; introduces document-focused Prisma models in a new schema file while removing them from the original schema; adds a DB migration to add Agreement.contentType. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as AgreementSheet (UI)
participant API as /api/teams/{teamId}/agreements
participant DB as Database
rect rgb(239,246,255)
note over UI: User selects content type
User->>UI: Choose LINK or TEXT
UI-->>User: Show URL input or Textarea
end
alt LINK
UI->>UI: Validate URL (agreementUrlSchema)
UI-->>User: Show url error if invalid
else TEXT
UI->>UI: Ensure non-empty textContent
UI-->>User: Show error if empty
end
UI->>API: POST { name, contentType, content, requireName }
API->>API: Zod validate & sanitize content
API->>DB: Create Agreement(name, contentType, content, requireName)
DB-->>API: Created
API-->>UI: Success
UI->>UI: Reset form and mutate list
sequenceDiagram
actor Viewer
participant View as *View Components*
participant AF as AccessForm
participant AS as AgreementSection
Viewer->>View: Open protected link
View->>AF: Render with agreementContentType = link.agreement?.contentType
AF->>AS: agreementContentType, agreementName, agreementContent
alt agreementContentType == "TEXT"
AS-->>Viewer: Render plain text (preserve whitespace)
else
AS-->>Viewer: Render anchor (agreementName → URL)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/view/access-form/index.tsx (1)
167-175
: Fix: TEXT agreements won’t render due to agreementName gate.AgreementSection is not shown when contentType is TEXT and agreementName is absent.
- {requireAgreement && agreementContent && agreementName ? ( + {requireAgreement && + agreementContent && + (agreementContentType === "TEXT" || !!agreementName) ? ( <AgreementSection {...{ data, setData, brand }} agreementContent={agreementContent} agreementName={agreementName} agreementContentType={agreementContentType} useCustomAccessForm={useCustomAccessForm} /> ) : null}components/links/link-sheet/agreement-panel/index.tsx (2)
135-153
: Guard against missing teamId and avoid non-null assertions.
teamId!
can produce runtime errors and bad URLs.try { setIsLoading(true); const contentType = currentFile.type; const supportedFileType = getSupportedContentType(contentType); + if (!teamId) { + toast.error("No active team found."); + return; + }And below:
- const { type, data, numPages, fileSize } = await putFile({ + const { type, data, numPages, fileSize } = await putFile({ file: currentFile, - teamId: teamId!, + teamId, });And:
- const response = await createAgreementDocument({ + const response = await createAgreementDocument({ documentData, - teamId: teamId!, + teamId, numPages, });
155-162
: Harden link extraction and avoid hard-coded domain.Handle missing links defensively and build the URL from config/origin.
- if (response) { - const document = await response.json(); - const linkId = document.links[0].id; - setData((prevData) => ({ - ...prevData, - link: "https://www.papermark.com/view/" + linkId, - })); - } + if (response) { + const doc = await response.json(); + const linkId = doc?.links?.[0]?.id; + if (!linkId) { + toast.error("No link generated for the uploaded agreement."); + } else { + const baseUrl = + process.env.NEXT_PUBLIC_APP_URL || + (typeof window !== "undefined" ? window.location.origin : ""); + setData((prev) => ({ + ...prev, + link: `${baseUrl}/view/${linkId}`, + })); + } + }
🧹 Nitpick comments (10)
components/view/access-form/index.tsx (1)
56-59
: Tighten the prop type to a string union.Avoids accidental typos and aligns with backend constraint.
- agreementContentType?: string; + agreementContentType?: "LINK" | "TEXT";components/links/link-sheet/agreement-panel/index.tsx (6)
63-69
: Type the local state and narrow contentType.Avoid implicit any and lock contentType to the union.
- const [data, setData] = useState({ + const [data, setData] = useState<{ + name: string; + link: string; + textContent: string; + contentType: "LINK" | "TEXT"; + requireName: boolean; + }>({ name: "", link: "", textContent: "", contentType: "LINK", requireName: true });
91-99
: Reset validation state on close.Clear URL validation state to avoid stale UI when reopening.
const handleClose = (open: boolean) => { setIsOpen(open); setData({ name: "", link: "", textContent: "", contentType: "LINK", requireName: true }); + setUrlError(""); + setIsUrlValid(true); setCurrentFile(null); setIsLoading(false); if (onClose) { onClose(); } };
226-231
: Consider aligning submit DTO with a shared type.If a shared AgreementCreateRequest exists, import it to prevent drift.
Would you like a quick shared type stub I can add under types/api.ts?
251-260
: Deduplicate close/reset logic; call handleClose(false).Keeps behavior consistent and ensures onClose runs.
- setIsLoading(false); - setIsOpen(false); - setData({ - name: "", - link: "", - textContent: "", - contentType: "LINK", - requireName: true - }); + setIsLoading(false); + handleClose(false);
378-379
: Clarify accepted upload types to match validation.UI currently suggests many types, but handler allows only PDF/Word.
- <Label>Or upload an agreement</Label> + <Label>Or upload an agreement (PDF or Word)</Label>
430-433
: Optional: disable submit on empty LINK.Relying solely on required works, but pre-emptive disable improves UX.
disabled={ - (data.contentType === "LINK" && !isUrlValid && data.link.trim() !== "") || + (data.contentType === "LINK" && (!data.link.trim() || !isUrlValid)) || (data.contentType === "TEXT" && !data.textContent.trim()) }prisma/schema/document.prisma (3)
69-70
: Fix truncated comment.- pageNumber Int // e.g., 1, 2, 3 for + pageNumber Int // e.g., 1, 2, 3 for page numbering
72-72
: Correct typo in comment (originalHeight).- metadata Json? // This will store the page metadata: {originalWidth: 100, origianlHeight: 100, scaledWidth: 50, scaledHeight: 50, scaleFactor: 2} + metadata Json? // This will store the page metadata: {originalWidth: 100, originalHeight: 100, scaledWidth: 50, scaledHeight: 50, scaleFactor: 2}
31-34
: Name pluralization (optional).
uploadedDocument
is an array; consideruploadedDocuments
for clarity (breaking change).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
components/links/link-sheet/agreement-panel/index.tsx
(7 hunks)components/view/access-form/agreement-section.tsx
(1 hunks)components/view/access-form/index.tsx
(3 hunks)components/view/dataroom/dataroom-document-view.tsx
(1 hunks)components/view/dataroom/dataroom-view.tsx
(1 hunks)components/view/document-view.tsx
(1 hunks)pages/api/teams/[teamId]/agreements/index.ts
(1 hunks)prisma/migrations/20250903000000_add_agreement_contenttype/migration.sql
(1 hunks)prisma/schema/document.prisma
(1 hunks)prisma/schema/schema.prisma
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/links/link-sheet/agreement-panel/index.tsx (2)
context/team-context.tsx (1)
useTeam
(85-85)components/document-upload.tsx (1)
DocumentUpload
(25-202)
components/view/access-form/agreement-section.tsx (2)
components/view/access-form/index.tsx (1)
DEFAULT_ACCESS_FORM_TYPE
(20-26)lib/utils/determine-text-color.ts (1)
determineTextColor
(24-28)
🔇 Additional comments (6)
components/view/dataroom/dataroom-document-view.tsx (1)
298-316
: LGTM: Propagates agreementContentType correctly to AccessForm.components/view/document-view.tsx (1)
254-272
: LGTM: agreementContentType is forwarded as expected.components/view/dataroom/dataroom-view.tsx (1)
239-257
: LGTM: agreementContentType plumbed through to AccessForm.components/links/link-sheet/agreement-panel/index.tsx (1)
391-415
: Safe HTML-escaped rendering for TEXT agreement content confirmed
AgreementSection renders{agreementContent}
inside a<span>
, so React’s JSX interpolation automatically escapes any HTML.prisma/schema/document.prisma (2)
82-85
: Enum looks good.Default to VERCEL_BLOB is sensible; no issues.
1-1
: Remove manual merge step Multi-file schema composition is auto-managed by Prisma when theprismaSchemaFolder
preview feature is enabled in yourgenerator
; all.prisma
files inprisma/schema
are merged duringprisma generate
,validate
,migrate
, etc. (prismagraphql.com, prisma.io)
@@ -50,15 +52,21 @@ export default function AgreementSheet({ | |||
isOnlyView = false, | |||
onClose, | |||
}: { | |||
defaultData?: { name: string; link: string; requireName: boolean } | null; | |||
defaultData?: { name: string; link: string; requireName: boolean; contentType?: string; textContent?: string } | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Strongly type contentType ('LINK' | 'TEXT') to prevent typos.
Use a literal union instead of string for safety across the app.
- defaultData?: { name: string; link: string; requireName: boolean; contentType?: string; textContent?: string } | null;
+ defaultData?: { name: string; link: string; requireName: boolean; contentType?: "LINK" | "TEXT"; textContent?: string } | null;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
defaultData?: { name: string; link: string; requireName: boolean; contentType?: string; textContent?: string } | null; | |
defaultData?: { | |
name: string; | |
link: string; | |
requireName: boolean; | |
contentType?: "LINK" | "TEXT"; | |
textContent?: string; | |
} | null; |
🤖 Prompt for AI Agents
In components/links/link-sheet/agreement-panel/index.tsx around line 55, the
defaultData prop types use contentType?: string which is error-prone; change
that field to a literal union contentType?: 'LINK' | 'TEXT' to enforce allowed
values, update any local interfaces or type aliases accordingly, and then fix
any usages/assignments across the file (or imported callers) to use the 'LINK'
or 'TEXT' literals so the compiler catches typos.
requireName: defaultData?.requireName || true, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean defaulting bug: false gets coerced to true.
|| true
overrides an explicit false. Use nullish coalescing.
- requireName: defaultData?.requireName || true,
+ requireName: defaultData?.requireName ?? true,
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
requireName: defaultData?.requireName || true, | |
}); | |
requireName: defaultData?.requireName ?? true, | |
}); |
🤖 Prompt for AI Agents
In components/links/link-sheet/agreement-panel/index.tsx around lines 84-85, the
expression "requireName: defaultData?.requireName || true" incorrectly coerces
an explicit false to true; replace the logical-or with nullish coalescing so it
respects false—i.e. use "defaultData?.requireName ?? true" (ensure optional
chaining remains and the result is typed/handled as a boolean).
const response = await fetch(`/api/teams/${teamId}/agreements`, { | ||
method: "POST", | ||
headers: { | ||
"Content-Type": "application/json", | ||
}, | ||
body: JSON.stringify({ | ||
...data, | ||
}), | ||
body: JSON.stringify(submitData), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Guard POST against missing teamId.
Same concern as upload flow.
Add before fetch:
+ if (!teamId) {
+ toast.error("No active team found.");
+ return;
+ }
Run to confirm no other !
assertions remain:
🏁 Script executed:
#!/bin/bash
rg -nP --type tsx -C1 '\bteamId!?\b'
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for non-null assertions on teamId in .ts and .tsx files
rg -nP '\bteamId\s*!' -g '*.ts' -g '*.tsx'
Length of output: 4058
Add guard for missing teamId in agreement-panel
In components/links/link-sheet/agreement-panel/index.tsx you still have non-null assertions on teamId at lines 137, 151 and the fetch at 233. Add before each usage:
+ if (!teamId) {
+ toast.error("No active team found.");
+ return;
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const response = await fetch(`/api/teams/${teamId}/agreements`, { | |
method: "POST", | |
headers: { | |
"Content-Type": "application/json", | |
}, | |
body: JSON.stringify({ | |
...data, | |
}), | |
body: JSON.stringify(submitData), | |
}); | |
if (!teamId) { | |
toast.error("No active team found."); | |
return; | |
} | |
const response = await fetch(`/api/teams/${teamId}/agreements`, { | |
method: "POST", | |
headers: { | |
"Content-Type": "application/json", | |
}, | |
body: JSON.stringify(submitData), | |
}); |
🤖 Prompt for AI Agents
In components/links/link-sheet/agreement-panel/index.tsx around lines 137, 151
and 233–239, you are using non-null assertions on teamId and calling fetch with
teamId without guarding for it; add an explicit guard before each usage (e.g.,
if (!teamId) { handle the missing teamId path — return early, disable the
action, or surface an error) so you never call fetch or access teamId when it's
undefined, and update UI/error handling accordingly to avoid runtime exceptions.
prisma/migrations/20250903000000_add_agreement_contenttype/migration.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pages/api/teams/[teamId]/agreements/index.ts (2)
14-23
: Harden schema: trim, enforce http/https for LINK, and cap TEXT length.As-is,
" "
passes min(1) then becomes empty after.trim()
. Also, LINK allowsjavascript:
(XSS vector) and very long TEXT is unbounded.Apply this diff to the schema:
-const createAgreementSchema = z.object({ - name: z - .string() - .min(1, "Name is required") - .max(150, "Name must be less than 150 characters"), - content: z.string().min(1, "Content is required"), - contentType: z.enum(["LINK", "TEXT"]).default("LINK"), - requireName: z.boolean().default(false), -}); +const createAgreementSchema = z + .object({ + name: z + .string() + .trim() + .min(1, "Name is required") + .max(150, "Name must be less than 150 characters"), + content: z.string().trim().min(1, "Content is required"), + contentType: z.enum(["LINK", "TEXT"]).default("LINK"), + requireName: z.boolean().default(false), + }) + .superRefine((val, ctx) => { + if (val.contentType === "LINK") { + try { + const url = new URL(val.content); + if (url.protocol !== "http:" && url.protocol !== "https:") { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["content"], + message: "Only http/https URLs are allowed for LINK contentType.", + }); + } + } catch { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["content"], + message: "Invalid URL for LINK contentType.", + }); + } + } else { + // TEXT + if (val.content.length > 5000) { + ctx.addIssue({ + code: z.ZodIssueCode.too_big, + maximum: 5000, + type: "string", + inclusive: true, + path: ["content"], + message: "Text content exceeds 5000 characters.", + }); + } + } + });
113-115
: Sanitize TEXT; normalize LINK.Sanitizing a URL string doesn’t prevent javascript: or data: schemes. Use sanitization only for TEXT and persist a normalized URL for LINK.
Apply this diff:
- // Sanitize content using existing sanitization logic - const sanitizedContent = validateContent(content); + // Sanitize/normalize by contentType + const sanitizedContent = + contentType === "TEXT" + ? validateContent(content) + : new URL(content).toString();
🧹 Nitpick comments (2)
pages/api/teams/[teamId]/agreements/index.ts (2)
111-112
: Minor: Schema default makes fallback unnecessary downstream.Since defaults are applied by Zod, all fields are present and normalized here.
119-122
: Drop redundant default and rely on validated/normalized values.Zod already defaulted contentType to "LINK"; also avoid storing post-trim empty names.
Apply this diff:
- name: name.trim(), - content: sanitizedContent, - contentType: contentType || "LINK", + name, + content: sanitizedContent, + contentType,If you prefer always-trimmed name/content at the DB edge, keep
.trim()
but pair it with the schema.trim()
to ensure min(1) still holds after trimming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/api/cron/domains/route.ts
(1 hunks)pages/api/teams/[teamId]/agreements/index.ts
(2 hunks)
🔇 Additional comments (4)
app/api/cron/domains/route.ts (1)
41-43
: ```shell
#!/bin/bashLocate the domain route file
FILE=$(find . -type f -path '*/app/api/cron/domains/route.ts' | head -n 1)
echo "Found file: $FILE"Print lines around the slug filter for review
sed -n '1,200p' "$FILE"
</blockquote></details> <details> <summary>pages/api/teams/[teamId]/agreements/index.ts (3)</summary><blockquote> `4-4`: **LGTM on Zod adoption.** Importing Zod is a solid step toward consistent validation. --- `10-10`: **LGTM on sanitization import.** Centralizing sanitization is good; see follow-up to scope it to TEXT only. --- `102-110`: **Good switch to safeParse with structured 400s.** Error shape is clear and client-friendly. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Add support for text content in agreements to allow users to display compliance information directly as a paragraph.
This enhancement extends the existing agreement system, which previously only supported external links. Now, users can choose to provide a direct text paragraph for their agreements, which will be displayed prominently to end-users. This involves updates to the database schema, agreement creation/editing UI, agreement display UI, and the API.
Slack Thread
Summary by CodeRabbit
New Features
Enhancements
Chores